bandwidth: use regexp to handle tc output and add IPv6 support #83572

chendotjs · 2019-10-07T14:54:34Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

fix newly-added 'chain N' and 'not_in_hw' output from 'tc filter show dev XXX'
use regexp to extract info we want from tc output, instead of splitting the output line

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix a bug where kubenet fails to parse the tc output.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2019-10-07T14:54:42Z

Hi @chendotjs. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chendotjs · 2019-10-07T15:01:24Z

/assign @dcbw @danwinship

danwinship

Definitely like the idea

danwinship · 2019-10-07T15:22:51Z

pkg/util/bandwidth/linux.go

 		// expected tc line:
 		// class htb 1:1 root prio 0 rate 1000Kbit ceil 1000Kbit burst 1600b cburst 1600b
-		if len(parts) != 14 {
-			return -1, fmt.Errorf("unexpected output from tc: %s (%v)", scanner.Text(), parts)
+		classShowMatcher := regexp.MustCompile(`class\shtb\s(1:\d+)\sroot.*rate\s`)


Why are you matching the "root.*rate" part too? Does that avoid some false positive? (If so maybe update the comment to indicate the kind of line you're trying to avoid matching too.)
Also, I'm not sure that using "\s" rather than just " " makes this any more generic/future-proof, and it makes it harder to read... (That applies throughout the patch.)
Also, you could make all of the MustCompile regexes be top-level variables so they only get compiled once rather than getting recompiled every time

Since the code uses command like tc class add dev xxx parent 1: classid 1:x htb rate 1kbit to add a new class where rate is an explicit parameter, so "root.*rate" ought to be matched explicitly. This is my primitive idea.
Other advice really make sense, thanks

danwinship · 2019-10-07T15:30:56Z

pkg/util/bandwidth/linux.go

 			// expected tc line:
 			// match <cidr> at <number>
-			if len(parts) != 4 {
-				return nil, fmt.Errorf("unexpected output: %v", parts)
+			cidrMatcher := regexp.MustCompile(`match\s([0-9a-f]{8}\/[[0-9a-f]{8}).*\s(\d+)`)


I'm not sure this is really an improvement over the old code. Also, it only handles IPv4 (the two hex strings are {32}, not {8} for IPv6; perhaps the test case should be updated for that). Also, there's no need to put ()s around the \d+ since you're not using it.

Currently IPv6 is not yet supported I suppose? Because the code uses command like tc filter add dev xxx protocol ip parent 1:0 prio 1 u32 match ip dst 1.2.3.4/32 flowid 1:2, the SELECTOR after match is ip, not ip6

ah... TestHexCIDR and TestAsciiCIDR test IPv6 addresses and the old code would have matched either IPv4 or IPv6 so I assumed we cared. If not then I guess someone will fix this when they add IPv6 support here. (Although I don't know if anyone realized that the bandwidth code doesn't currently support IPv6. @khenidak do you know if this is being tracked?)

I'm quite sure that IPv6 is not supported currently and I'll fix it.

alejandrox1 · 2019-10-07T16:37:01Z

/ok-to-test
/milestone v1.17
/priority important-soon

alejandrox1 · 2019-10-07T16:38:12Z

/remove-priority critical-urgent

markjacksonfishing · 2019-11-08T15:05:46Z

@MrHohn @freehan @chendotjs We are fast approaching code freeze on 11/14. Hopefully we can get this in 1.17?

markjacksonfishing · 2019-11-10T17:00:59Z

Circling back on this @chendotjs @MrHohn @freehan, code freeze is 11/14 and by 5 pm pst I need to make the call if this gets moved out of this milestone.

markjacksonfishing · 2019-11-13T11:11:03Z

I have had no response to this and code freeze is tomorrow. I have to move this to milestone v1.18

markjacksonfishing · 2019-11-13T11:11:16Z

/milestone v1.18

dcbw · 2020-01-09T21:43:02Z

@chendotjs can you rebase to fix the conflict? thanks!

chendotjs · 2020-01-10T11:10:20Z

@dcbw commits have been rebased and squashed.

jtslear · 2020-02-12T12:41:44Z

Hello @danwinship and @dcbw,
Bug Triage team here for the 1.18 release. This is a friendly reminder that code freeze is scheduled for 5 March. Is this PR still intended for milestone 1.18?

jtslear · 2020-02-28T16:52:21Z

Hello @danwinship and @dcbw,
Bug Triage team here for the 1.18 release. This is a friendly reminder that code freeze is scheduled for 5 March. We are waiting review on this PR, there's been no movement on this since mid January.

cc @kubernetes/sig-network-pr-reviews

danwinship · 2020-02-29T23:24:36Z

The IPv6 support here seems extremely complicated and I feel like it ought to be possible to simplify it a lot.

I would recommend just reverting back to the original version of the PR, addressing the non-IPv6-related comments, and resubmitting that, and then we can think about IPv6 support again later.

fix newly-added 'chain N' output from 'tc filter show dev XXX'

chendotjs · 2020-03-03T15:33:02Z

Truly thanks for your practical advice ! @danwinship
This pr has been reverted and follows the non-IPv6-related comments, ptal if you have time.

danwinship · 2020-03-04T14:20:33Z

/lgtm
/approve

k8s-ci-robot · 2020-03-04T14:23:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendotjs, danwinship

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/util/bandwidth/OWNERS~~ [danwinship]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cadmuxe · 2020-03-04T18:21:03Z

/retest

k8s-ci-robot requested review from andrewsykim and m1093782566 October 7, 2019 14:56

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 7, 2019

chendotjs mentioned this pull request Oct 7, 2019

bandwidth: handle new 'chain X' output from 'tc filter show dev XYZ' #80059

Closed

k8s-ci-robot assigned danwinship and dcbw Oct 7, 2019

danwinship reviewed Oct 7, 2019

View reviewed changes

chendotjs force-pushed the tc-fix branch 2 times, most recently from 7e204f5 to 1749e71 Compare October 7, 2019 16:34

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 7, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 7, 2019

k8s-ci-robot removed the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Oct 7, 2019

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 21, 2019

k8s-ci-robot assigned freehan and MrHohn Nov 5, 2019

k8s-ci-robot modified the milestones: v1.17, v1.18 Nov 13, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2020

chendotjs force-pushed the tc-fix branch from bc84ef2 to 6ab9c1c Compare January 10, 2020 02:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2020

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 14, 2020

chendotjs force-pushed the tc-fix branch from 6ab9c1c to ef1a3c2 Compare January 22, 2020 02:17

bandwidth: use regexp to handle tc output

ef5c920

fix newly-added 'chain N' output from 'tc filter show dev XXX'

chendotjs force-pushed the tc-fix branch from ef1a3c2 to ef5c920 Compare March 3, 2020 12:36

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2020

k8s-ci-robot merged commit e4e3d72 into kubernetes:master Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bandwidth: use regexp to handle tc output and add IPv6 support #83572

bandwidth: use regexp to handle tc output and add IPv6 support #83572

chendotjs commented Oct 7, 2019 •

edited

k8s-ci-robot commented Oct 7, 2019

chendotjs commented Oct 7, 2019

danwinship left a comment

danwinship Oct 7, 2019

chendotjs Oct 7, 2019

danwinship Oct 7, 2019

chendotjs Oct 7, 2019

danwinship Oct 7, 2019

chendotjs Oct 18, 2019

alejandrox1 commented Oct 7, 2019 •

edited

alejandrox1 commented Oct 7, 2019

markjacksonfishing commented Nov 8, 2019

markjacksonfishing commented Nov 10, 2019

markjacksonfishing commented Nov 13, 2019

markjacksonfishing commented Nov 13, 2019

dcbw commented Jan 9, 2020

chendotjs commented Jan 10, 2020

jtslear commented Feb 12, 2020

jtslear commented Feb 28, 2020

danwinship commented Feb 29, 2020

chendotjs commented Mar 3, 2020

danwinship commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020

cadmuxe commented Mar 4, 2020

bandwidth: use regexp to handle tc output and add IPv6 support #83572

bandwidth: use regexp to handle tc output and add IPv6 support #83572

Conversation

chendotjs commented Oct 7, 2019 • edited

k8s-ci-robot commented Oct 7, 2019

chendotjs commented Oct 7, 2019

danwinship left a comment

Choose a reason for hiding this comment

danwinship Oct 7, 2019

Choose a reason for hiding this comment

chendotjs Oct 7, 2019

Choose a reason for hiding this comment

danwinship Oct 7, 2019

Choose a reason for hiding this comment

chendotjs Oct 7, 2019

Choose a reason for hiding this comment

danwinship Oct 7, 2019

Choose a reason for hiding this comment

chendotjs Oct 18, 2019

Choose a reason for hiding this comment

alejandrox1 commented Oct 7, 2019 • edited

alejandrox1 commented Oct 7, 2019

markjacksonfishing commented Nov 8, 2019

markjacksonfishing commented Nov 10, 2019

markjacksonfishing commented Nov 13, 2019

markjacksonfishing commented Nov 13, 2019

dcbw commented Jan 9, 2020

chendotjs commented Jan 10, 2020

jtslear commented Feb 12, 2020

jtslear commented Feb 28, 2020

danwinship commented Feb 29, 2020

chendotjs commented Mar 3, 2020

danwinship commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020

cadmuxe commented Mar 4, 2020

chendotjs commented Oct 7, 2019 •

edited

alejandrox1 commented Oct 7, 2019 •

edited